Snowflake identifier #5
Conversation
|
@roxblnfk This is now passing after those couple of PR's were merged. Feedback welcomed |
src/SnowflakeDiscord.php
Outdated
| private int $workerId = 0, | ||
| private int $processId = 0, |
There was a problem hiding this comment.
Looks like these fields shouldn't be hardcoded in attributes.
We can declare it as null|int<0, max> and use hardcoded values if they were specified.
Need to think how to provide this options from config (where ENVs can be used) to behavior implementation.
There was a problem hiding this comment.
As I was writing these classes this did cross my mind. For instance SnowflakeInstagram and shardId is something developers will want more control over, if using the identifiers to their full potential/purpose.
The same applies for UUID's when specifying a node or localDomain.
I moved on pretty quick but the idea I had was trivial, a static class allowing developers to define values for these behaviors at a time which makes sense to them. E.g. application bootstrap, after passing configuration, environment variables etc. The behavior class constructors would be modified, accepting and defaulting to a null value. When not specified getListenerArgs would use a value from the <Identifier>Defaults class.
Just an idea, open to others.
final class SnowflakeDiscordDefaults
{
private static int $workerId = 0;
private static int $processId = 0;
public static function getWorkerId(): int
{
return self::$workerId;
}
public static function setWorkerId(int $workerId): void
{
self::$workerId = $workerId;
}
public static function getProcessId(): int
{
return self::$processId;
}
public static function setProcessId(int $processId): void
{
self::$processId = $processId;
}
}
final class SnowflakeDiscord extends BaseSnowflake
{
public function __construct(
string $field = 'snowflake',
?string $column = null,
private ?int $workerId = null,
private ?int $processId = null,
bool $nullable = false,
) {
}
protected function getListenerArgs(): array
{
return [
'field' => $this->field,
'workerId' => $this->workerId === null ? SnowflakeDiscordDefaults::getWorkerId() : $this->workerId,
'processId' => $this->processId === null ? SnowflakeDiscordDefaults::getProcessId() : $this->processId,
'nullable' => $this->nullable,
];
}
}There was a problem hiding this comment.
Implemented similar to above, however defaults are considered within __construct instead of getListenerArgs. Done for all relevant identifiers. @roxblnfk Please review/resolve when you get a moment keen to 🚢 and wrap up an initial implementation.
There was a problem hiding this comment.
Unsure why there's a delay in merging? If something additional or changes are needed, let me know.
|
|
||
| $modifier->setTypecast( | ||
| $registry->getEntity($this->role)->getFields()->get($this->field), | ||
| [$factory, 'createFromInteger'], |
There was a problem hiding this comment.
ORM schema should be serializable. That's why a static factory should be used here. Is it possible?
There was a problem hiding this comment.
I think the ramsey/identifier package has taken a different direction and moved away from static classes/factories. Not a bad thing, but it likely means we’ll need to create our own static classes/methods for instantiating these factories 🥲
While I wasn’t aware of the serialisation requirement or that it was an issue, the way I originally implemented this seemed the most logical at the time, given some factories require constructor arguments.
There was a problem hiding this comment.
Sorry not at my desk at moment, it’s 3:20am here in Australia. But I’ll throw out any idea.
An option might be: define a static method within each behaviour class. E.g
public static function fromInteger(array $args);
Then there’s the issue of needing to supply the ramsey factories with behaviour values into its constructor. Perhaps we could support a third value in the array?
[self::class, 'fromInteger', [1, 2]
Or allow the callback method to be defined similar to how enum annotations are declared.
[self::class, 'fromInteger(1, 2)']
Obviously this would require some passing, and changes so that the supplied parameters are captured and provided back to the static method when invoked.
Would something like that work?
There was a problem hiding this comment.
I think the ramsey/identifier package has taken a different direction and moved away from static classes/factories. Not a bad thing, but it likely means we’ll need to create our own static classes/methods for instantiating these factories 🥲
It's interesting why there is any state if we want to load already generated IDs. We won't know the state of generators for those IDs that are in the database in advance.
There was a problem hiding this comment.
It's interesting why there is any state if we want to load already generated IDs. We won't know the state of generators for those IDs that are in the database in advance.
I'm not seeing an issue when generating a schema. Or am I am misunderstanding the problem?
return [
'account_role' => [
...
Schema::TYPECAST => [
'id' => [
unserialize('O:51:\"Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\":6:{s:73:\"\0Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\0clockSequenceCounter\";i:0;s:66:\"\0Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\0nodeIdShifted\";i:0;s:64:\"\0Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\0epochOffset\";i:0;s:59:\"\0Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\0nodeId\";i:0;s:58:\"\0Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\0clock\";O:43:\"Ramsey\\Identifier\\Service\\Clock\\SystemClock\":0:{}s:61:\"\0Ramsey\\Identifier\\Snowflake\\GenericSnowflakeFactory\0sequence\";O:54:\"Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\":6:{s:68:\"\0Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\0defaultState\";s:12:\"c52375de3d50\";s:61:\"\0Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\0clock\";O:43:\"Ramsey\\Identifier\\Service\\Clock\\SystemClock\":0:{}s:61:\"\0Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\0cache\";O:45:\"Ramsey\\Identifier\\Service\\Cache\\InMemoryCache\":4:{s:52:\"\0Ramsey\\Identifier\\Service\\Cache\\InMemoryCache\0cache\";a:0:{}s:57:\"\0Ramsey\\Identifier\\Service\\Cache\\InMemoryCache\0defaultTtl\";i:120;s:56:\"\0Ramsey\\Identifier\\Service\\Cache\\InMemoryCache\0cacheSize\";i:100;s:52:\"\0Ramsey\\Identifier\\Service\\Cache\\InMemoryCache\0clock\";O:43:\"Ramsey\\Identifier\\Service\\Clock\\SystemClock\":0:{}}s:65:\"\0Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\0precision\";E:53:\"Ramsey\\Identifier\\Service\\Clock\\Precision:Millisecond\";s:68:\"\0Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\0initialValue\";N;s:72:\"\0Ramsey\\Identifier\\Service\\Clock\\MonotonicClockSequence\0initialValueUsed\";b:0;}}'),
'createFromInteger',
],
'accountId' => 'int',
'parentId' => 'int',
'name' => 'string',
'description' => 'string',
'locked' => 'bool',
'createdAt' => 'datetime',
'updatedAt' => 'datetime',
],
],
];There was a problem hiding this comment.
In the schema, there should be no function calls, especially unserialize. It should be an array that can be packed into a text cache format, like JSON.
Ideally, we need static factories.
There was a problem hiding this comment.
Thanks for clarifying.
Image below depicts some changes to Cycle\ORM\Parser\Typecast which I think might aid in working around this problem with serialization.
Then within the base classes, instead of something like:
$factory = $this->snowflakeFactory();
$modifier->setTypecast(
$registry->getEntity($this->role)->getFields()->get($this->field),
[$factory, 'createFromInteger'],
);You might do:
$modifier->setTypecast(
$registry->getEntity($this->role)->getFields()->get($this->field),
[self::class, 'fromInteger', $this->getListenerArgs()],
);Where the fromInteger method looks something like.
public static function fromInteger(
int|string $identifier,
DatabaseInterface $database,
array $arguments
): \Ramsey\Identifier\Snowflake
{
return (new GenericSnowflakeFactory(
$arguments['node'],
$arguments['epochOffset']
))->createFromInteger($identifier);
}All theory and completely untested. Just wanted to run the idea by you first before taking it any further.
🔍 What was changed
🔔 Note
Pipelines will fail until the following PR's are merged and released.
📝 Checklist
💯 Test coverage
🟢 Psaml with empty baseline